Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sink: Kafka message length and batch size limit for Open Protocol #1079

Merged
merged 21 commits into from
Nov 24, 2020
Merged

sink: Kafka message length and batch size limit for Open Protocol #1079

merged 21 commits into from
Nov 24, 2020

Conversation

liuzix
Copy link
Contributor

@liuzix liuzix commented Nov 16, 2020

What problem does this PR solve?

  • Open Protocol messages risk exceeding the message length limit for Kafka.
  • Users cannot control the number of records in a single message produced by Open Protocol.

What is changed and how it works?

  • The Open Protocol Encoder reads the max-message-bytes parameter from the sink-uri and respects this limit.
  • The Open Protocol Encoder now reads max-batch-size from opts and respects this limit.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has interface methods change

Side effects

  • Possible performance regression
  • Increased code complexity

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation

Release note

  • No release note

@liuzix liuzix added component/open-protocol Open TiCDC protocol component. component/sink Sink component. labels Nov 16, 2020
@liuzix liuzix added this to the v4.0.9 milestone Nov 16, 2020
@liuzix
Copy link
Contributor Author

liuzix commented Nov 16, 2020

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Nov 16, 2020

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Nov 16, 2020

/run-all-tests

2 similar comments
@liuzix
Copy link
Contributor Author

liuzix commented Nov 16, 2020

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Nov 17, 2020

/run-all-tests

amyangfei
amyangfei reviewed Nov 18, 2020
View reviewed changes
cdc/sink/codec/json.go Outdated Show resolved Hide resolved
kafka_consumer/main.go Outdated Show resolved Hide resolved
@liuzix
Copy link
Contributor Author

liuzix commented Nov 19, 2020

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Nov 19, 2020

/run-all-tests

1 similar comment
@liuzix
Copy link
Contributor Author

liuzix commented Nov 19, 2020

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Nov 19, 2020

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Nov 19, 2020

/run-all-tests

2 similar comments
@liuzix
Copy link
Contributor Author

liuzix commented Nov 20, 2020

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Nov 20, 2020

/run-all-tests

amyangfei
amyangfei reviewed Nov 20, 2020
View reviewed changes
Copy link
Contributor

@amyangfei amyangfei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 20, 2020
zier-one
zier-one reviewed Nov 20, 2020
View reviewed changes
kafka_consumer/main.go Outdated Show resolved Hide resolved
kafka_consumer/main.go Show resolved Hide resolved
@liuzix
Copy link
Contributor Author

liuzix commented Nov 20, 2020

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Nov 23, 2020

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Nov 23, 2020

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Nov 23, 2020

/run-all-tests

1 similar comment
@liuzix
Copy link
Contributor Author

liuzix commented Nov 23, 2020

/run-all-tests

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Nov 24, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Nov 24, 2020
@zier-one zier-one merged commit 2958b02 into pingcap:master Nov 24, 2020
@zier-one zier-one added the needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. label Nov 24, 2020
@zier-one
Copy link
Contributor

/run-cherry-picker

ti-srebot pushed a commit to ti-srebot/ticdc that referenced this pull request Nov 24, 2020
@liuzix @ti-srebot
cherry pick pingcap#1079 to release-4.0
4249bcf 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Nov 24, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #1112

zier-one pushed a commit that referenced this pull request Nov 24, 2020
@ti-srebot
sink: Kafka message length and batch size limit for Open Protocol (#1079
a643787 
) (#1112)
@amyangfei amyangfei mentioned this pull request Nov 24, 2020
Closed
@liuzix liuzix mentioned this pull request Nov 24, 2020
Merged
9 tasks
@amyangfei amyangfei mentioned this pull request Dec 1, 2020
Closed
@ti-srebot ti-srebot mentioned this pull request Dec 21, 2020
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amyangfei amyangfei amyangfei left review comments

@zier-one zier-one zier-one approved these changes

@ti-srebot ti-srebot ti-srebot approved these changes

@3pointer 3pointer Awaiting requested review from 3pointer

Assignees
No one assigned
Labels
component/open-protocol Open TiCDC protocol component. component/sink Sink component. needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
v4.0.9
Development

Successfully merging this pull request may close these issues.
4 participants
@liuzix @zier-one @ti-srebot @amyangfei